Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36849 - Run GHA on Ruby 3.0 #9989

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

ofedoren
Copy link
Member

@ofedoren ofedoren commented Jan 10, 2024

@ekohl, there is another, more general and detailed approach trying to fix the same issue. I hope the code and the comments will help to understand the idea of what's going wrong.

Unfortunately, that's the last thing I could find. The third and (that's only my opinion) the last option would be to completely get rid of that weird app/models/host.rb module, which (per my understanding) simply tries to simulate an abstract class.

UPD: TIL that rubocop can apparently yell that the indentation of a comment can be wrong and that's unacceptable 🤦

Alternative to #9871.

app/models/host/base.rb Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a quick look at this and it appears the approach is to make the constructor's signature match the base class. That makes sense to me.

As for STI, I'm not sure we can get rid of it. I'd like to. At this moment I think we only have 2 implementations, so be sure to check out what Discovery does.

app/models/concerns/foreman/sti.rb Outdated Show resolved Hide resolved
test/helpers/application_helper_test.rb Outdated Show resolved Hide resolved
@ofedoren
Copy link
Member Author

ofedoren commented Jan 12, 2024

Even though initially I wanted to get the (any) fix merged at first, and then take a look at the plugins which fail, but after I've tested with foreman_discovery it seems some additional fixes might be needed in this/other PR as well, so I'll rather start a list here with verified plugins which support this PR:

@ofedoren ofedoren force-pushed the foreman-on-ruby-3-letsgooo branch from c6dd66a to d49fd7e Compare January 12, 2024 14:09
@@ -1,5 +1,5 @@
{
"postgresql": ["12"],
"ruby": ["2.7"],
"ruby": ["2.7", "3.0"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we separate this out to its own commit? I'd like to merge all the preparation work before we pull the trigger.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some deprecation warnings when running tests:

2024-01-23T18:54:00.9931233Z Mocha deprecation warning at /home/runner/work/foreman/foreman/app/services/proxy_api/dhcp.rb:36:in `unused_ip': Expectation defined at /home/runner/work/foreman/foreman/test/unit/proxy_api/dhcp_test.rb:99:in `block in <class:ProxyApiDhcpTest>' expected positional hash ({:query => "mac=00:11:22:33:44:55"}), but received keyword arguments (:query => "mac=00:11:22:33:44:55"). These will stop matching when strict keyword argument matching is enabled. See the documentation for Mocha::Configuration#strict_keyword_argument_matching=.
2024-01-23T18:54:01.0281310Z Mocha deprecation warning at /home/runner/work/foreman/foreman/app/services/proxy_api/dhcp.rb:36:in `unused_ip': Expectation defined at /home/runner/work/foreman/foreman/test/unit/proxy_api/dhcp_test.rb:74:in `block in <class:ProxyApiDhcpTest>' expected positional hash ({:query => ""}), but received keyword arguments (:query => ""). These will stop matching when strict keyword argument matching is enabled. See the documentation for Mocha::Configuration#strict_keyword_argument_matching=.
2024-01-23T18:54:01.0407752Z Mocha deprecation warning at /home/runner/work/foreman/foreman/app/services/proxy_api/dhcp.rb:36:in `unused_ip': Expectation defined at /home/runner/work/foreman/foreman/test/unit/proxy_api/dhcp_test.rb:90:in `block in <class:ProxyApiDhcpTest>' expected positional hash ({:query => "from=192.168.0.50&to=192.168.0.150"}), but received keyword arguments (:query => "from=192.168.0.50&to=192.168.0.150"). These will stop matching when strict keyword argument matching is enabled. See the documentation for Mocha::Configuration#strict_keyword_argument_matching=.
2024-01-23T18:54:13.7290895Z /home/runner/work/foreman/foreman/vendor/bundle/ruby/2.7.0/gems/fog-vsphere-3.6.3/lib/fog/vsphere/compute.rb:344: warning: deprecated Object#=~ is called on Integer; it always returns nil
2024-01-23T18:54:13.7458370Z /home/runner/work/foreman/foreman/vendor/bundle/ruby/2.7.0/gems/fog-vsphere-3.6.3/lib/fog/vsphere/compute.rb:344: warning: deprecated Object#=~ is called on Integer; it always returns nil
2024-01-23T18:54:22.8953091Z Mocha deprecation warning at /home/runner/work/foreman/foreman/app/models/host/managed.rb:715:in `ipmi_boot': Expectation defined at /home/runner/work/foreman/foreman/test/models/host_test.rb:3245:in `block (2 levels) in <class:HostTest>' expected keyword arguments (:function => "bootdevice", :device => "bios"), but received positional hash ({:function => "bootdevice", :device => "bios"}). These will stop matching when strict keyword argument matching is enabled. See the documentation for Mocha::Configuration#strict_keyword_argument_matching=.

The fog-vsphere warnings will break with Ruby 3.2. The other ones look like some things we should fix in our code and then enable strict_keyword_argument_matching in mocha.

@ekohl
Copy link
Member

ekohl commented Jan 24, 2024

Can we also enable strict keyword argument matching (https://mocha.jamesmead.org/Mocha/Configuration.html#strict_keyword_argument_matching=-instance_method)? Perhaps make that the whole second commit?

@ofedoren ofedoren force-pushed the foreman-on-ruby-3-letsgooo branch 2 times, most recently from ecb8a73 to 047dceb Compare January 24, 2024 13:24
@ofedoren
Copy link
Member Author

Done. Not sure though if we need that. As there was stated, this will be true by default anyway.

@ofedoren ofedoren force-pushed the foreman-on-ruby-3-letsgooo branch from 047dceb to b17c9c8 Compare January 24, 2024 13:27
@ofedoren ofedoren changed the title Run GHA on Ruby 3.0 (alternative 2.0) Fixes #36849 - Run GHA on Ruby 3.0 Jan 24, 2024
@evgeni
Copy link
Member

evgeni commented Jan 24, 2024

Done. Not sure though if we need that. As there was stated, this will be true by default anyway.

That way we don't accidentally regress now that we fixed the offenders until it's true in the future?

@ofedoren ofedoren force-pushed the foreman-on-ruby-3-letsgooo branch from b17c9c8 to eefdccd Compare January 24, 2024 13:51
@ekohl
Copy link
Member

ekohl commented Jan 24, 2024

Indeed. If it was true today then these tests wouldn't have failed. This way we also force plugins to adopt.

@ofedoren
Copy link
Member Author

Some Katello test failures are related to strict keyword argument matching I guess, but otherwise 🍏 for Foreman.

@ekohl
Copy link
Member

ekohl commented Jan 24, 2024

Some Katello test failures are related to strict keyword argument matching I guess, but otherwise 🍏 for Foreman.

I've love to leverage what you did in #9989 (comment) and see how other repositories behave.

@ofedoren
Copy link
Member Author

I've love to leverage what you did in #9989 (comment) and see how other repositories behave.

I'm preparing more PRs regarding what I found in Katello, I'll paste them here once they are ready. The plan to go over my locally checkedout other plugins as well.

@ekohl ekohl merged commit 4728839 into theforeman:develop Feb 1, 2024
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants